[target-allocator] Populate store assets (authorization information) for Prometheus CR watcher #1710
Conversation
Add context parameter to to LoadConfig method in perparation to use K8s client in the method. Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Add K8s client and logger, in preparation for obtaining service / pod monitor sotore assets. Signed-off-by: Matej Gera <matej.gera@coralogix.com>
…tors Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
matej-g
force-pushed
the
prometheus-cr-auth
branch
from
4dc5200 to
d17ed35
Compare
| // based on the service monitor and endpoints specs. | ||
| // This code borrows from | ||
| // https://github.com/prometheus-operator/prometheus-operator/blob/06b5c4189f3f72737766d86103d049115c3aff48/pkg/prometheus/resource_selector.go#L73. | ||
| func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor( |
There was a problem hiding this comment.
It looks like the method you linked is a public method, are we unable to use it?
There was a problem hiding this comment.
ah maybe because we don't want to make a whole resource selector. Interested in your perspective here :)
There was a problem hiding this comment.
I was considering use those public methods, but eventually decided to borrow some of the code, because 1) as you said we would need to build the whole resource selector; 2) the methods for monitors seem to do extra stuff (whole selection and validation of monitors), whereas here we wanted to just populate the store.
However, now that you point this out, and as I noticed these methods were recently exported, it might be a good opportunity to replace this whole part of the watcher logic directly with the resource selector methods. We could get that extra validation etc. for free, but perhaps first I'd ensure those methods will result in correct selection of monitors for our purposes.
What do you think about doing this as a follow up work?
There was a problem hiding this comment.
that's a great idea for some followup – i love anything that allows us to delete code :)
There was a problem hiding this comment.
Let's do it 👍 I'll open an issue for this to ensure it won't go unaddressed.
|
@jaronoff97 forgot to add in the description - in addition to adding unit tests, I also tested this locally on a test cluster, with expected results, see resulting scrape config for an example service monitor with basic auth (checked in browser):
|
|
@matej-g awesome! how does this work with secret mounting? does the collector need to mount a secret volume or does the TA need to? Thinking we may want to update docs to reflect this. |
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
| } | ||
| return labels.SelectorFromSet(s) | ||
| } | ||
|
|
||
| // getInformers returns a map of informers for the given resources. | ||
| func getInformers(factory informers.FactoriesForNamespaces) (map[string]*informers.ForResource, error) { |
There was a problem hiding this comment.
thought: we should open an issue for adding in the new scrape config CRD and maybe the probe CRD too.
There was a problem hiding this comment.
Agreed 👍 I'll open an issue for this.
| // based on the service monitor and endpoints specs. | ||
| // This code borrows from | ||
| // https://github.com/prometheus-operator/prometheus-operator/blob/06b5c4189f3f72737766d86103d049115c3aff48/pkg/prometheus/resource_selector.go#L73. | ||
| func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor( |
There was a problem hiding this comment.
that's a great idea for some followup – i love anything that allows us to delete code :)
Signed-off-by: Matej Gera <matejgera@gmail.com>
|
@matej-g looks like a bad git merge 😅 |
|
Yup, forgot to save 🤦, should be good now though. Seems like there's an unrelated failure in the test (https://github.com/open-telemetry/opentelemetry-operator/actions/runs/5324102167/jobs/9642911432?pr=1710#step:5:1268) |
|
@matej-g could you please rebase? |
TylerHelmuth
added
the
ready-to-merge
|
cc @open-telemetry/operator-ta-maintainers this should finally be good to merge, thank you 🙇 |
…for Prometheus CR watcher (open-telemetry#1710) * Adjust Watcher interface Add context parameter to to LoadConfig method in perparation to use K8s client in the method. Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Enhance the Prometheus CR watcher struct Add K8s client and logger, in preparation for obtaining service / pod monitor sotore assets. Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Adds methods and logic to obtain store assets from service / pod monitors Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add unit tests for Promtehus CR watcher Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add changelog Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add disclaimer about secrets in the readme Signed-off-by: Matej Gera <matejgera@gmail.com> * Fix store param and tests after branch update Signed-off-by: Matej Gera <matejgera@gmail.com> * Fix botched merge Signed-off-by: Matej Gera <matejgera@gmail.com> --------- Signed-off-by: Matej Gera <matej.gera@coralogix.com> Signed-off-by: Matej Gera <matejgera@gmail.com> Co-authored-by: Jacob Aronoff <jaronoff97@users.noreply.github.com> Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Co-authored-by: Pavol Loffay <p.loffay@gmail.com>
Resolves #1669.
This change adds logic to populate the so-called store assets - this includes information from service and pod monitors that relates to authorization information (e.g. basic auth, bearer token, TLS) - in order to propagate these information to scrape configs. Currently, this logic is not implement, resulting in scrape configurations missing proper credentials.
This PR also adds tests for the
LoadConfigmethod for the Prometheus CR watcher.